-
-
Notifications
You must be signed in to change notification settings - Fork 591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(replace): prevent accidental replacement within assignment #798
Conversation
Thanks for this PR. Could you go into some more detail as to how this change works and the problem that it solve? Examples would be wonderful. (This will be important information for context for folks that may be looking at this PR via blame) |
@shellscape This PR will prevent replacing strings where they are followed by a single equals sign. For example, where the plugin is called as follows: replace({
values: {
'process.env.DEBUG': 'false'
}
}) Observe the following code: // Input
process.env.DEBUG = false
if (process.env.DEBUG == true) {
//
}
// Before this PR
false = false // this throws an error because false cannot be assigned to
if (false == true) {
//
}
// After this PR
process.env.DEBUG = false
if (false == true) {
//
} |
Thanks for the explanation. That's some pretty specific behavior, and not one that I'm sure we want to enable by default. That's one of those "ecosystem gotchas" that I'm sure you're aware of: users rely on a specific behavior and structure code accordingly, only to have that behavior "fixed" in a patch version, causing chaos. I see the value in this change, and it was also raised earlier today in #803. This is a pretty big change that could break some behaviors that folks are relying on, and I wouldn't want to hamstring those folks with no way back, so to speak. That said, I think it'd be reasonable to make this an option of some sort, defaulting to the original behavior. I would even be willing to output a warning if the option was |
@shellscape That seems very reasonable. I've added a |
What is the process of getting an npm package deployed with new feature changes like this one? This feature is relevant to some issues I'm facing while trying to replace |
@acmiyaguchi patience. you exercise patience. please refrain from similar replies in the future, it just creates noise. |
I like this change a lot, but I do have a suggestion: Because users will now receive a warning if they do not explicitly configure this option, the examples in README should be updated to show explicit configuration of this option in all cases. Someone following the current examples will be surprised at the warning. |
@bdkjones good suggestion. would you be willing to open a PR? |
@shellscape Sure. I would use |
Rollup Plugin Name:
@rollup/replace
This PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
Description
It's possible when replacing
process.env
values that packages may be writing to these values. This PR adds a negative lookahead test to the generated RegExp to prevent some of these situations.